feat: add GroupColumn support for Float16 in multi-column GROUP BY#23785
Open
tohuya6 wants to merge 1 commit into
Open
feat: add GroupColumn support for Float16 in multi-column GROUP BY#23785tohuya6 wants to merge 1 commit into
tohuya6 wants to merge 1 commit into
Conversation
…umn GROUP BY `multi_group_by::group_column_supported_type` gates which GROUP BY columns can use the column-wise `GroupValuesColumn` fast path. Any unsupported column forces the entire grouping onto the byte-encoded `GroupValuesRows` fallback, so a single `Float16` key dragged an otherwise-qualifying multi-column GROUP BY onto the slow path. `Float16` reuses the existing `PrimitiveGroupValueBuilder` with no new builder type: its native `half::f16` already implements the `HashValue` canonicalization (`hash_float!(f16, f32, f64)`), so `-0.0` / `+0.0` folding and NaN grouping match Float32 / Float64 with no extra handling. - accept `Float16` in `group_column_supported_type` and dispatch it in `make_group_column` - move `Float16` from the rejected to the accepted set in the `group_column_supported_type` <-> `make_group_column` consistency fuzz, and repoint the two tests that used `Float16` as their stock "unsupported" example to a permanently-invalid unit combo (`Time64(Second)`), so they stay stable as sibling primitive builders (Decimal256, Interval, ...) land - add an end-to-end unit test (Float16 GROUP BY dedups including nulls, folds -0.0/+0.0 into one group stored as +0.0, groups equal NaNs, and preserves the Float16 output type) and a Float16 GROUP BY block in aggregate.slt - add a `(Float16, Int32)` group-count benchmark to `benches/multi_group_by.rs` (capped below f16's ~63.5k distinct finite values) Part of apache#22715
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
multi_group_by::group_column_supported_typegates which GROUP BY columns mayuse the column-wise
GroupValuesColumnfast path, and the gate isall-or-nothing: a single unsupported column forces the entire grouping onto
the byte-encoded
GroupValuesRowsfallback, even when every other key columnwould have qualified. A
Float16key triggers exactly that today.Float16reuses the existingPrimitiveGroupValueBuilderwith no new buildertype: its native
half::f16already implements theHashValuecanonicalization (
hash_float!(f16, f32, f64)), so-0.0/+0.0folding andNaNgrouping matchFloat32/Float64with no extra handling.What changes are included in this PR?
Float16ingroup_column_supported_typeand dispatch it inmake_group_column.Float16from the rejected to the accepted set in thegroup_column_supported_type↔make_group_columnconsistency fuzz, andrepoint the two tests that used
Float16as their stock "unsupported" exampleto a permanently-invalid unit combo (
Time64(Second)), so they stay stable assibling primitive builders (Decimal256, Interval, …) land independently.
(Float16, Int32)group-count benchmark tobenches/multi_group_by.rs(capped below f16's ~63.5k distinct finite values).
Are these changes tested?
Yes.
test_group_values_column_float16: aFloat16key stays on theGroupValuesColumnpath, dedups including nulls, folds-0.0/+0.0into onegroup (stored as
+0.0), groups equalNaNs, and round-trips with theFloat16output type preserved.Float16routes through the dispatcher.Float16GROUP BYcoverage inaggregate.slt.Are there any user-facing changes?
No API changes.
GROUP BYqueries with aFloat16key now use the column-wisefast path instead of the row-encoded fallback; results are unchanged.